feat(cohorts): Amplitude cohort sync endpoints and sync keys - #8290
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughAdds Bearer-token authentication and environment-scoped lifecycle management for cohort sync keys. Adds Amplitude cohort creation and member add/remove endpoints. Persists source types, records audit events, updates membership state, and documents the API and security scheme. Adds unit tests for authentication, validation, environment isolation, membership updates, audit records, and key revocation. Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟠 High · up to The new Amplitude cohort-sync endpoints can acknowledge requests that later fail to apply membership changes, especially for oversized identifiers or removals, unavailable synchronization services, transaction timing failures, or audit-write failures; the API schema also does not fully match runtime requirements. These are high-impact correctness and data-consistency risks, so the PR is not ready to merge without fixes or explicit acceptance. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
api
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8290 +/- ##
==========================================
+ Coverage 98.64% 98.79% +0.14%
==========================================
Files 1609 1614 +5
Lines 64570 64988 +418
==========================================
+ Hits 63696 64204 +508
+ Misses 874 784 -90 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
feature
github-actions
Bot
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
openapi.yaml (1)
1661-1737: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBearer authentication is still undocumented for the Amplitude cohort-sync operations.
The three operations
api_v1_cohort_sync_amplitude_lists_create,api_v1_cohort_sync_amplitude_lists_add_create, andapi_v1_cohort_sync_amplitude_lists_remove_createhave nosecurityfield.CohortSyncKeyAuthenticationrequiresAuthorization: Bearer <sync-key>for these endpoints, per the PR objectives.A prior review thread on this file flagged the identical issue and recorded it as resolved by a commit that added a "Cohort Sync Key" security-scheme extension in
api/api/openapi.py, with the note that CI regenerates this rootopenapi.yamlautomatically from that source. In this file version, the fix has not taken effect:
- Lines 1661-1737 still show no
securityfield on the three operations.- The
securitySchemesblock (around line 29354) still lists onlyEnvironment API Key,Master API Key,basicAuth, andtokenAuth. No "Cohort Sync Key" scheme is defined.Confirm that the authentication extension still exists and is wired to these operations, and that the generation step that produces this file actually runs before merge.
#!/bin/bash # Description: Verify the Cohort Sync Key OpenAPI authentication extension exists and is applied. set -euo pipefail echo '--- Check for the authentication extension in api/api/openapi.py ---' fd -a openapi.py api/api | xargs -r rg -n -C 5 'CohortSyncKeyAuthentication|OpenApiAuthenticationExtension|Cohort Sync Key' echo '--- Check CohortSyncKeyAuthentication usage in cohorts sync views ---' fd -a sync_views.py api/cohorts | xargs -r rg -n -C 3 'authentication_classes|CohortSyncKeyAuthentication' echo '--- Check sdk/openapi.yaml for the scheme ---' rg -n -C 3 'Cohort Sync Key' sdk/openapi.yaml || echo 'not found in sdk/openapi.yaml'
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 94df6b2d-a068-4e8b-a583-933430a093e8
📒 Files selected for processing (7)
api/cohorts/services.pyapi/cohorts/sync_views.pyapi/cohorts/views.pyapi/tests/unit/cohorts/test_sync_views.pyapi/tests/unit/cohorts/test_views.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mdopenapi.yaml
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
github-actions
Bot
added
feature
github-actions
Bot
added
docs
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Overall great! Just wondering whether we could gate on the org plan and have the cohort creation and audit in the same transaction to avoid zombie cohort and revert entirely in case of failure
github-actions
Bot
added
feature
2 participants
docs/if required so people know about the feature.Changes
Contributes to Flagsmith/flagsmith-private#260.
The receiving side of Amplitude's list-based cohort sync contract, feeding the existing membership ledger. Spec copied from Create a cohort sync integration and Receiving behavioral cohorts — routes, field names and status codes are fixed by Amplitude.
CohortSyncKey: per-environment bearer credential, hashed at rest, plaintext returned once; managed at/environments/<key>/cohorts/sync-keys/./cohort-sync/amplitude/lists/creates the cohort and returnslist_id;lists/<uuid>/add|remove/turn{"user_ids": [...]}into pending ledger rows for the existing apply task.How did you test this code?
Unit tests cover the auth matrix, list creation, add/remove reaching (moto) DynamoDB via the apply task, the 404 fences, and key management.
Beyond that, this can only really be tested once Amplitude grants us Integration Portal access: the endpoints are exercised for real by Amplitude's own calls, first through the portal's testing tab and
Release internallymode, then in production.